Add error bundle support to translate-c, unify cmdTranslateC and cImport
#25495
Conversation
|
Thanks for working on this!
As soon as it lands in main branch there, it can be backported here. |
kcbanner
force-pushed
the
fixup_translate_c
branch
from
f181e81 to
730590b
Compare
|
I decided to reduce the scope of this PR, and just include the translate-c / cImport changes. I'll make a separate PR to backport the translate-c and arocc changes I made recently (ziglang/translate-c#199, Vexu/arocc#895, Vexu/arocc#897), once they are both merged. Those don't need to block this PR. I noticed that the |
… error bundles on stdout) via --zig-integration - Revive some of the removed cache integration logic in `cmdTranslateC` now that `translate-c` can return error bundles - Fixup inconsistent path separators (on Windows) when building the aro include path - Move some error bundle logic from resinator into aro.Diagnostics - Add `ErrorBundle.addRootErrorMessageWithNotes` (extracted from resinator)
…llowed by .off or .warning - translate-c: emit `file_system_inputs` even in the case of failure, if available - translate-c: fixup emitting zero-length `file_system_inputs`
…lateC` - Add std.zig.Server.allocErrorBundle, replace duplicates
kcbanner
force-pushed
the
fixup_translate_c
branch
from
730590b to
8b6cdc3
Compare
kcbanner
changed the title
translate-c error handling, cache integration, and #include_next fixes translate-c, unify cmdTranslateC and cImport
…g the current error - Compilation: renameTmpIntoCache doesn't need to be pub after the `translateC` change
| const aro = @import("aro"); | ||
| const ErrorBundle = std.zig.ErrorBundle; | ||
|
|
||
| pub fn aroDiagnosticsToErrorBundle( |
There was a problem hiding this comment.
I won't block the merge on this, but in the interest of avoiding "util" in namespaces I wonder if @Vexu would consider accepting this logic to live inside aro.Diagnostics. I see that aro already takes advantage of std.zig so it doesn't seem unreasonable to me.
Also won't block the merge on this, but the more flexible API is to take a *ErrorBundle.Wip and add the errors into it, rather than returning it as a return value. That's similar to how it's better to accept an array list and append to it rather than returning a fresh one.
There was a problem hiding this comment.
Naming that util file may have been the hardest part of this PR :D
I originally actually did place this in aro.Diagnostics.toErrorBundle, however there was a chicken / egg problem with the other ErrorBundle changes in this PR that toErrorBundle would have depended on in the aro repo - so I would have had to PR those changes separately here, PRed the toErrorBundle change to aro, then backported it here.
If Vexu is alright with it, then I can make that change to aro after this PR is merged, and make your suggested API change as well. Then I can backport that aro change once it lands.
There was a problem hiding this comment.
I'm OK with it as long as you add a test for it since it'll be otherwise unused there.
There was a problem hiding this comment.
Sounds good, I will PR it there once this is merged.
| .errors = error_bundle, | ||
| }; | ||
| }, | ||
| else => fatal("unexpected message type received from translate-c: {s}", .{@tagName(header.tag)}), |
There was a problem hiding this comment.
FYI
| else => fatal("unexpected message type received from translate-c: {s}", .{@tagName(header.tag)}), | |
| else => fatal("unexpected message type received from translate-c: {t}", .{header.tag}), |
|
I'll let @squeek502 have another look but as far as I'm concerned it's ready to land. |
4 participants
Closes #25393
Closes #25431
--zig-integration. I followed a similar approach to what resinator does here.cmdTranslateCnow thattranslate-ccan return error bundlesCompilation.translateCand rework bothcmdTranslateCandcImportto use thisErrorBundle.addRootErrorMessageWithNotes(extracted from resinator)Remaining TODOs:
cImportto use--zig-integration(to fully resolve translate-c prints to stderr rather than sending error bundle to parent process, resulting in terminal warnings #25431)